-
Notifications
You must be signed in to change notification settings - Fork 382
[dotnet-trace] Add collect-linux verb #5570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
c09b095 to
0989d21
Compare
Extensions houses provider composition helpers, instead of literal extension methods for strings. Centralize all provider parsing + composition logic and keep Profile as a data container.
76e65e1 to
61b2569
Compare
61b2569 to
c8e53ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good. Aside from some small comments inline a few broader things:
- Historically we relied on manual testing to keep these tools operating well but now that we have less time available from the testers I think we need to improve our automated testing. Partly this is to ensure we aren't inadvertently changing the original 'collect' verb and partly to ensure going forward the 'collect-linux' behavior doesn't regress either. I think the best way to do this would be:
- Open a new PR that we'll check in first containing some basic tests of the existing collect verb.
- Commit this PR 2nd and all the tests in the 1st PR should continue to pass. This ensures we didn't change anything unintended.
To do the testing we probably need to create some small interface shims. We already have an IConsole interface defined that could be moved to the shared Common folder. We could also create a small interface around the DiagnosticsClient.EventPipeCollect() API so that a test can return some dummy data in a stream instead. dotnet-counters has some example tests that show how we can run some code and then confirm the console output is what we expect. In this case I imagine we'd be running the Collect() function and giving some chosen input arguments.
- I think there is a bit more adjustment to be done on some of the output text, but it should be fine to get this one in first, then tweak afterwards in some 3rd PR.
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
| private static Stopwatch s_stopwatch = new(); | ||
| private static LineRewriter s_rewriter = new() { LineToClear = Console.CursorTop - 1 }; | ||
| private static LineRewriter s_rewriter; | ||
| private static bool s_printingStatus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If testing caused multiple instances of CollectLinuxCommandHandler to run in parallel, would we want those instances to be sharing any of these static fields? I'm guessing we'd want all these fields to be instanced too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to non-static. From some learning, I'm seeing that parallelization occurs at the test class level, so it doesn't seem like the theory members would run in parallel.
| LinuxHeader, | ||
| LinuxProfile("cpu-sampling"), | ||
| "" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be some output about where the test output is being written to and telling the user to press Enter to stop the trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the output file to Collect-linux, thanks for the catch. As per the press enter/ctrl+c message, normally that is part of the status printing.
For collect-linux, I added the line rewriting/status printing to the callback, so to mock that for the tests, extended the RecordTraceInvoker test seam to invoke the callback once with the progress output type, causing the status+ Press Enter/Ctrl+C to emit once.
eff3e5f to
40c5905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments inline around the error handling, but if you want to handle that in a separate PR I'm fine with it. In terms of checking this in, lets make sure Juan gets the release out before this gets checked in.
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
…eption To swallow stacktraces from expected invalid cli option combinations without swallowing stacktraces from other ArgumentExceptions thrown, swap to throwing the tools common custom CommandLineErrorException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got one last suggestion to adjust the preview banner text, but otherwise I think we are looking good 👍
| } | ||
|
|
||
| Console.WriteLine("=========================================================================================="); | ||
| Console.WriteLine("The collect-linux verb is in preview. Some usage scenarios may not yet be supported,"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt the average user will know what "NetTrace V6" means :) They probably recognize the file suffix but not the "V6". I'd also suggest we can make the message a little useful by telling them what will work and pointing them at the docs. To keep the message from getting too long we can put bug filing info as part of a longer message we include in the docs. How about this:
"The collect-linux verb is a new preview feature and relies on an updated version of the .nettrace file format. The latest PerfView release supports these trace files but other ways of using the trace file may not work yet. See the docs at https://learn.microsoft.com/dotnet/core/diagnostics/dotnet-trace for more details."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, I've been separating the lines to where each line is just under 90 characters, which is the width of
Provider Name Keywords Level Enabled By
so slightly adjusted
|
The test failure is #2541 |
Implements dotnet/docs#47894
Following the addition of emitting native runtime and custom EventSource events as user_events through dotnet/runtime#115265 and the public release of https://github.com/microsoft/one-collect which supports collecting both .NET user_events and Linux perf events into a single .nettrace file,
dotnet-tracewill support a new verb,collect-linux, that wraps aroundrecord-trace.This PR does the following:
collect-linuxverb and serializes a subset ofdotnet-trace collectoptions in addition to acollect-linuxspecific--perf-eventsoption intorecord-traceargs. (see [Diagnostics][dotnet-trace] Add collect-linux verb docs#47894 for overarching details)dotnet-tracecpu-sampling->dotnet-common+dotnet-sampled-thread-time) and addscollect-linuxspecific profileslist-profilesverb with revamped profiles + multiline description formattingEventPipeProvidercomposition logic (MergeProfileAndProviders+ToProviders->ComputeProviderConfig) and renameExtensions.cs->ProviderUtils.csProviderParsing.cs->ProviderCompositionTests.cs)collectlogic + expanddotnet-tracecommon optionsTesting
dotnet-trace collect-linux
On Linux
collect-linux
collect-linux --help
`collect-linux` without elevated privileges
`collect-linux` with elevated privileges
On Windows (and I presume other non-Linux OS):
`collect-linux`
dotnet-trace list-profiles
`list-profiles`
dotnet-trace profiles: dotnet-common - Lightweight .NET runtime diagnostics designed to stay low overhead. Includes GC, AssemblyLoader, Loader, JIT, Exceptions, Threading, JittedMethodILToNativeMap, and Compilation events Equivalent to --providers "Microsoft-Windows-DotNETRuntime:0x100003801D:4". dotnet-sampled-thread-time (collect) - Samples .NET thread stacks (~100 Hz) toestimate how much wall clock time code is using. gc-verbose - Tracks GC collections and samples object allocations. gc-collect - Tracks GC collections only at very low overhead. database - Captures ADO.NET and Entity Framework database commands cpu-sampling (collect-linux) - Kernel CPU sampling events for measuring CPU usage. thread-time (collect-linux) - Kernel thread context switch events for measuring CPU usage and wall clock time